Should-fix (flagged by both reviewers): catch block can double-decrement expectation counters.

If doSpawnSession throws before line 642 mutates deadPersona.sessionId, this calls onDesignParticipantDisconnect(oldSessionId). The handler finds the same persona again (sessionId still matches), sees it's in retriedPersonas, and decrements questionsExpected/proposalsExpected/refinementExpected a second time for the same dead persona. This can push counters negative.

Conversely, if spawn succeeds (sessionId was mutated), this calls disconnect with the new sessionId — which works correctly (falls through to retry-exhausted path). But the asymmetry is subtle and the failure path is buggy.

Suggestion: capture the old sessionId before the try block and use it to guard against re-entry, or restructure so the catch doesn't re-enter the disconnect handler.

Should-fix: in-place sessionId mutation loses the old session ID.

Two consequences:

1. If the old session's process is still lingering (delayed teardown), cleanupDesignSessions iterates state.personas and only sees the new sessionId — the old process leaks.
2. Late-arriving disconnect events for the old sessionId won't match any persona in state.personas, so they're silently ignored. This is benign but makes debugging harder.

Consider keeping the old ID (e.g., a previousSessionIds set on the persona) or treating the replacement as a distinct entry.

Should-fix (flagged by both reviewers): removing the > 0 guards enables premature phase transitions.

The old code had state.questionsExpected > 0 && (and similarly for proposals/refinement). With the guard removed, if the last persona dies and retry is exhausted, questionsExpected decrements to 0, and 0 >= 0 triggers the transition. The aliveCount === 0 cancellation check should catch this first — but it won't if another persona is still alive (aliveCount > 0) while this counter hits 0. That advances the phase with zero expectations from this counter.

Suggestion: restore the > 0 guards, or move the aliveCount === 0 cancellation to before the per-phase counter adjustment.

Should-fix: refinementExpected++ without checking if the dead persona already responded.

When the first crash triggers the retry path (line 718-719), the early return skips the refinementExpected-- that normally runs. So the count is still inflated by the dead persona's slot. Then here, refinementExpected++ adds another slot for the replacement — net +1 over what it should be. If the dead persona had already responded before crashing, the design now waits for an extra response that may never come (timeout).

Suggestion: check state.refinementRespondedIds.has(deadPersona.sessionId) or equivalent before incrementing.

Should-fix: aliveCount can be stale under concurrent persona crashes.

If two personas crash near-simultaneously and both retries are exhausted, each computes aliveCount === 1 (seeing the other as alive before its transport is torn down). Neither triggers the aliveCount === 0 cancellation, and the design hangs until timeout.

This is a narrow race but possible under load. A mitigation: recheck aliveCount just before the cancel decision, or move the cancel check into a shared function that recalculates.

Nit: activeDivergence is typed inline. Since refinementQueue presumably holds the same shape, consider referencing a shared Divergence type to avoid drift.

Nit (flagged by both reviewers): The name.replace('_', ' ') normalization duplicates logic from processNextDivergence. If the rule changes, both sites need updating. Consider extracting a helper or normalizing at the source (where activeDivergence.personas is populated).

Nit: activeDivergence is set here but never cleared (e.g., when refinement completes or the queue is exhausted). Currently safe because retryPersona checks state.phase === 'refinement' before using it, but clearing it explicitly would prevent future bugs.

Blocker (flagged by both reviewers): deadPersona.sessionId is mutated to the new session ID before the health check. If waitForBridge returns false, killSession runs, then the catch block calls onDesignParticipantDisconnect(deadPersona.sessionId) with the new (already-killed) session ID. The disconnect handler lookup may silently no-op if the kill already cleaned it from the registry, leaving the dead persona unaccounted for — expectations never decrement.

Fix: move this assignment after the waitForBridge success path (after line 649), or stash the original ID and pass it to the recursive onDesignParticipantDisconnect call.

Blocker (flagged by both reviewers): Removing the questionsExpected > 0 guard (and equivalently at lines 744, 755) allows 0 >= 0 to trigger a phase transition with zero contributions.

Scenario: persona A is alive and working, persona B (retry exhausted) disconnects. aliveCount > 0 so the cancellation path doesn't fire. questionsExpected decrements to 0, questionsReceived is 0, 0 >= 0 is true → phase advances with zero questions. Persona A's eventual submission arrives in the wrong phase and is silently dropped.

Fix: restore a > 0 guard (e.g. state.questionsExpected > 0 && state.questionsReceived >= state.questionsExpected), or ensure the all-dead check accounts for this edge.

Should-fix (flagged by both reviewers): activeDivergence is set here but never cleared when refinement for this divergence completes or when the phase changes. If a persona crashes after refinement finishes but before the next divergence (or during synthesis), retryPersona will see the stale divergence, send a refinement prompt, and increment refinementExpected — corrupting the counter.

Fix: set state.activeDivergence = undefined at the end of processNextDivergence (when all responses are in) or on phase transitions out of refinement.

Should-fix: retryPersona is fire-and-forget (void) and takes 30+ seconds (spawn + health check). During that time, timeouts can fire and advance the phase. The phase-specific prompt selection (lines 661-683) uses the phase at completion time, not at crash time — so a persona that crashed during questioning could receive an independent or refinement prompt if the phase advanced while retry was in-flight.

Also, if cancelDesign runs concurrently (user cancel or all-dead from another disconnect), the retry can finish spawning a session that cancel already iterated past, leaving an orphan.

Consider: (1) capture the phase at crash time and use it for prompt selection, (2) check phase before spawn in addition to after.

Should-fix: If between the persona's death (which decremented refinementExpected) and the retry completing here, refinementResponses >= refinementExpected became true and processNextDivergence already advanced, then this increment makes refinementExpected permanently higher than the number of responses that will arrive — the divergence counter is off.

Nit: This inline type { description: string; personas: string[]; impact: string } is presumably the same shape as divergence objects in refinementQueue. Extract a named type to keep them in sync.

Nit: String.replace with a string pattern only replaces the first occurrence. If persona names can contain multiple underscores (e.g. ux_design_lead), this won't match. Consider replaceAll('_', ' ') or a regex /_ /g. (Pre-existing at line 584 too.)

Blocker (bump): deadPersona.sessionId is mutated to the new session ID before waitForBridge. If the health check fails, killSession runs on the new session, then the catch calls onDesignParticipantDisconnect(deadPersona.sessionId) with the already-killed new ID — the disconnect handler may no-op (registry cleaned), leaving counters uncorrected.

Fix: move this assignment after the waitForBridge success path, or stash the original ID and use it in the catch.

Blocker (bump): Removing the questionsExpected > 0 guard (and equivalently for proposals/refinement at lines 744, 755) allows 0 >= 0 to trigger a phase transition with zero contributions.

This is reachable when combined with the counter-corruption race from the retryingPersonas finding below: a delayed disconnect can decrement counters while a retry is in-flight, pushing *Expected to 0 while a live persona hasn't yet submitted. Restoring the > 0 guard acts as a safety net against this edge case.

Should-fix (bump): aliveCount is computed once at the top but used later for the cancel decision. If two personas crash near-simultaneously and both retries are exhausted, each sees the other as alive before its transport tears down — neither triggers aliveCount === 0, and the design hangs until timeout.

Mitigation: recheck alive count just before the cancel decision.

Should-fix (bump): retryPersona is fire-and-forget and takes ~30s (spawn + health check). During that window, timeouts can fire and advance the phase. The phase-specific prompt at completion time may not match the phase at crash time.

Consider: (1) capture the phase at crash time for prompt selection, (2) check phase before spawn in addition to after.

Should-fix (new, flagged by both reviewers): retryingPersonas is written to (line 635 add, line 687 delete) but never read anywhere. If a delayed disconnect event for the old sessionId arrives while retryPersona is in-flight, the handler finds the persona, sees retriedPersonas.has(name) is true, and falls through to the retry-exhausted path — decrementing counters even though the retry might succeed.

Add state.retryingPersonas.has(persona.name) as a guard here to skip counter adjustment while a retry is in-flight. Otherwise the set is dead code and should be removed.

Should-fix (new, flagged by both reviewers): cancelDesign is async and fire-and-forget here. Between the void call and cancelDesign setting state.phase = 'cancelled', another disconnect event could enter the handler, see aliveCount === 0 again, and fire a second concurrent cancel.

Fix: set state.phase = 'cancelled' synchronously before the void cancelDesign(...) call:

state.phase = 'cancelled' as any // prevent re-entry
void cancelDesign(threadId, ...).catch(...)

Nit (bump): activeDivergence inline type { description: string; personas: string[]; impact: string } is the same shape as divergence objects in refinementQueue. Extract a shared Divergence type to prevent drift.

Blocker (bump, flagged by both reviewers): deadPersona.sessionId is mutated to the new session ID before waitForBridge. If the health check fails, killSession runs on the new session, then the catch calls onDesignParticipantDisconnect(deadPersona.sessionId) — but that's now the new session ID, not the old one. The old session's tmux/registry entry is never cleaned up (resource leak), and the disconnect handler looks up the wrong persona.

Additionally, the old session is never explicitly killed before spawning the replacement. Compare with the synthesizer timeout handler which kills the old session before respawning.

Fix: save the old session ID, kill the old session before spawning, and only assign the new session ID after the health check succeeds.

Blocker (bump, flagged by both reviewers): Removing the questionsExpected > 0 guard (and equivalently at lines 747, 758) allows 0 >= 0 to trigger a phase transition with zero contributions.

Scenario: 2 personas alive, both crash in the same phase. First crash triggers retry (returns early). Second crash hits the exhausted path, decrements expected to 0. 0 >= 0 is true → advances to the next phase with zero data.

The aliveCount === 0 check at line 728 only fires when retries are exhausted. If one persona is retrying and the other dies with retry exhausted, aliveCount may be 0 but the code reaches the phase-advance path. Restore the > 0 guard or add an explicit zero-expected check.

Should-fix (bump): aliveCount is computed once at the top but used later for the cancel decision. If two personas crash near-simultaneously and both retries are exhausted, each computes aliveCount === 1 (seeing the other as alive), so neither triggers the all-dead cancellation. Result: design stuck with no live personas.

Consider recomputing aliveCount at the point of use, or using an atomic decrement-and-check pattern.

Should-fix (bump, flagged by both reviewers): retryPersona is fire-and-forget (void) and takes ~30s (spawn + health check). During that window, timeouts can fire and advance the phase. The phase-specific prompt selection at the end of retryPersona (lines 659-680) reads state.phase which may have changed by then. The retried persona gets instructions for a phase that already ended.

Consider capturing the phase at retry start and comparing when the retry completes, or adding a phase-change check before sending the prompt.

Should-fix (bump): cancelDesign is async and fire-and-forget here. Between the void call and cancelDesign setting state.phase = 'cancelled', another disconnect event could enter onDesignParticipantDisconnect, pass the designs.get(threadId) check, and attempt its own cancel — causing double cleanup.

Consider setting state.phase = 'cancelled' synchronously before awaiting cancelDesign, or adding a guard at the top of cancelDesign for the cancelled phase.

Should-fix (new, flagged by ts-reviewer): When the retry fails, catch calls onDesignParticipantDisconnect(deadPersona.sessionId). But at that point retryingPersonas still has the persona name (the finally block hasn't run yet). The disconnect handler hits if (state.retryingPersonas.has(persona.name)) return and exits silently — no "retry exhausted" message, no expectation adjustment, no aliveCount === 0 check. The persona just vanishes.

Fix: delete from retryingPersonas before calling onDesignParticipantDisconnect in the catch block:

} catch (err) {
  process.stderr.write(`daemon: design: ${name} auto-retry failed: ${err}\n`)
  state.retryingPersonas.delete(name)
  onDesignParticipantDisconnect(deadPersona.sessionId)
}

And remove the delete from finally (or keep it as a no-op safety net).

Should-fix (new, flagged by both reviewers): The old persona session is never killed before spawning the replacement. The bridge disconnected (that's how we got here), but the tmux session and registry entry for the old sessionId may linger. This leaks resources.

Add explicit cleanup before spawning:

const oldInfo = registry.get(deadPersona.sessionId)
if (oldInfo && !killsInProgress.has(deadPersona.sessionId)) {
  await killSession(oldInfo, 'persona crashed, retrying').catch(() => {})
}

Nit (bump): activeDivergence inline type { description: string; personas: string[]; impact: string } is the same shape as divergence objects in refinementQueue. Extract a shared Divergence type to keep them in sync.

Blocker (bump, flagged by both reviewers): deadPersona.sessionId is mutated to the new session ID before waitForBridge. If the health check fails, killSession runs on the new session, then the catch at line 690 deletes retryingPersonas and calls onDesignParticipantDisconnect(deadPersona.sessionId) — but that's now the new (already-killed) session ID, not the original dead one. The disconnect handler either finds no matching persona or finds the stale entry, leading to expectations never being decremented and the phase getting stuck.

Acknowledged: old-session cleanup (line 638-641) and retryingPersonas guard (line 722) were added to address prior feedback — those are good. The core issue is the mutation ordering.

Fix: capture const originalSessionId = deadPersona.sessionId before line 643 and use it in the catch block. Move the mutation to after the health check succeeds.

Blocker (bump, flagged by both reviewers): Removing the questionsExpected > 0 guard (and equivalently at lines 752, 763) allows 0 >= 0 to trigger a phase transition with zero contributions.

Scenario: 2 personas, both crash in questioning phase. First crash triggers retry (returns early, no decrement). Second crash: retry exhausted → questionsExpected-- → now 0. 0 >= 0 is true → fires aggregateAndPostQuestions with an empty question set. The aliveCount === 0 cancel path fires cancelDesign with void (async), so the synchronous counter check runs first.

Fix: restore > 0 guards, or add if (aliveCount === 0) return before the counter-decrement blocks (the cancel already handles this case).

Should-fix (bump): aliveCount is computed once at line 724 but used later for the cancel decision at line 736. If two personas crash near-simultaneously and both retries are exhausted, each computes aliveCount === 1 (seeing the other as alive before its transport tears down) — neither triggers aliveCount === 0, and the design hangs with no personas.

Fix: recompute aliveCount at the cancel decision point, or set state.phase = 'cancelled' synchronously before the void cancelDesign(...) call so subsequent disconnect events short-circuit.

Should-fix (bump, flagged by both reviewers): retryPersona is fire-and-forget (void) and takes ~30s (spawn + health check). During that window, the phase timeout can fire and advance the phase. The phase-specific prompt at the end of retryPersona (lines 667-687) reads state.phase at completion time, not at crash time — so a persona that crashed during independent could receive a refinement prompt (or no prompt at all if the phase doesn't match).

Fix: capture const crashPhase = state.phase at call time and use it for prompt selection inside retryPersona.

Should-fix (bump): cancelDesign is async and fire-and-forget here. Between the void call and cancelDesign setting state.phase = 'cancelled', another disconnect event can enter the handler, see aliveCount === 0 again, and fire a second concurrent cancel — double cleanup, double user message.

Fix: set state.phase = 'cancelled' synchronously before void cancelDesign(...), so subsequent disconnect events see the cancelled state and bail early.

Should-fix (new, flagged by ts-reviewer): No early-exit guard for state.phase === 'cancelled' || 'complete' at the top of retryPersona before calling doSpawnSession. If the design is cancelled (e.g., by the user) while the persona crash is being processed, the retry still proceeds to spawn a new session, wait up to HEALTH_TIMEOUT_MS, and only then clean it up at line 658-662.

Fix: add if (state.phase === 'cancelled' || state.phase === 'complete') return before line 637.

Nit (new, flagged by both reviewers): replaceAll('_', ' ') here but processNextDivergence (pre-existing, line ~584) uses replace('_', ' ') which only replaces the first occurrence. If a persona name has multiple underscores (e.g. ux_design_lead), the match logic diverges. Both sites should use the same normalization — ideally extract a shared personaNameMatches(a, b) helper.

Nit (bump): This inline type is the same shape as divergence objects in refinementQueue. Extract a shared Divergence type to prevent drift.

Blocker (bump, flagged by both reviewers): deadPersona.sessionId is mutated to the new session ID before waitForBridge. If the health check fails, killSession runs on the new session, then the catch calls onDesignParticipantDisconnect(deadPersona.sessionId) — which is now the new ID. The fallback path works by accident (the new session was killed so transport.has returns false), but the mutation-then-throw pattern is fragile.

Fix: Defer mutation until after health check:

const newSessionId = result.sessionId
const ok = await waitForBridge(newSessionId, HEALTH_TIMEOUT_MS)
if (!ok) { /* kill newSessionId, throw */ }
deadPersona.sessionId = newSessionId // only now

Blocker (bump, flagged by both reviewers): Removing the questionsExpected > 0 guard (and equivalently at lines 756, 767) allows 0 >= 0 to trigger a phase transition with zero contributions.

The aliveCount === 0 cancel path at line 737 is meant to replace this guard, but under concurrent persona crashes + retry failures, aliveCount can be stale (see line 725 comment). If the cancel path doesn't fire, the 0 >= 0 fallthrough advances the phase with no participants.

Fix: Either restore the > 0 guards as belt-and-suspenders, or add an assertion documenting the invariant that this code is only reachable when aliveCount > 0.

Should-fix (bump): aliveCount is computed once here but used later for the cancel decision at line 737. If two personas crash near-simultaneously and both retries are exhausted, each computes aliveCount === 1 (seeing the other as alive), both skip the cancel path, and both decrement *Expected — potentially advancing the phase with zero participants.

Additionally, aliveCount doesn't exclude personas in retryingPersonas whose old sessions are dead but new sessions aren't in transport yet — this can cause premature "all personas dead" cancellation while a retry is in flight.

Fix: Recompute alive count at the cancel decision point, and exclude retryingPersonas from the dead count.

Should-fix (bump): retryPersona is fire-and-forget (void) and takes ~30s (spawn + health check). During that window, timeouts can fire and advance the phase. The phase-specific prompt at completion time (lines 668-687) reads state.phase and state.activeDivergence from live state — if the phase advanced while the retry was in flight, the persona gets a stale or wrong prompt.

Should-fix (bump): cancelDesign is async and fire-and-forget here. Between the void call and cancelDesign setting state.phase = 'cancelled', another disconnect event could enter onDesignParticipantDisconnect and also trigger cancel — double cleanup. Consider setting state.phase = 'cancelled' synchronously here before the void cancelDesign(...) call.

Should-fix (new, flagged by both reviewers): When a retried persona re-enters refinement, refinementExpected is not re-incremented. The disconnect handler already decremented it (line 766). If the retried persona responds, refinementResponses increases but the bookkeeping is off — the system may advance to the next divergence prematurely or count the response as unexpected.

Same issue in the independent phase (line 668): proposalsExpected was decremented by the disconnect handler but isn't re-incremented when the retry succeeds.

Fix: Re-increment refinementExpected / proposalsExpected when the retry succeeds and the persona is prompted.

Should-fix (new): If the design ends during retry (phase becomes cancelled/complete), this early return skips the state.retryingPersonas.delete(name) at line 694. The persona name remains permanently in retryingPersonas, which blocks future disconnect handling for that persona name.

Nit (bump): activeDivergence inline type { description: string; personas: string[]; impact: string } is presumably the same shape as divergence objects in refinementQueue. Extract a shared Divergence type to keep them in sync.

Nit (new, flagged by both reviewers): replaceAll('_', ' ') here vs replace('_', ' ') (first occurrence only) in processNextDivergence. Persona names with multiple underscores (e.g. ux_design_lead) would match here but not there. Consider extracting a matchesPersonaName(a, b) helper for consistency.